Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intoroduce query parameters with multiple values to Endpoint API #2631

Merged
merged 9 commits into from
Jan 24, 2024

Conversation

ekhov
Copy link
Contributor

@ekhov ekhov commented Jan 17, 2024

Introduce query codec for multi value query parameters. Now HttpCodec.Query[A] returns Chunk[A] as a result value. Add new query codecs to describe these multi value parameters to QueryCodecs preserving old ones(query etc), which are now a transformed versions of multi value codecs.

Now single value codecs fail if endpoint receives multiple values for this query

/claim #2321

@ekhov ekhov force-pushed the support-multi-value-query-params branch from 925532e to 8d0effc Compare January 17, 2024 19:18
@ekhov ekhov force-pushed the support-multi-value-query-params branch 2 times, most recently from e9276d5 to b096f41 Compare January 18, 2024 08:44
@jdegoes
Copy link
Member

jdegoes commented Jan 18, 2024

There is a Scala 3 build failure that is failing the build (maybe among other things).

@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (6930757) 65.38% compared to head (00bf4b9) 65.46%.

Files Patch % Lines
...ed/src/main/scala/zio/http/codec/QueryCodecs.scala 90.00% 1 Missing ⚠️
...scala/zio/http/codec/internal/EncoderDecoder.scala 91.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2631      +/-   ##
==========================================
+ Coverage   65.38%   65.46%   +0.07%     
==========================================
  Files         144      144              
  Lines        8407     8420      +13     
  Branches     1532     1494      -38     
==========================================
+ Hits         5497     5512      +15     
+ Misses       2910     2908       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ekhov ekhov force-pushed the support-multi-value-query-params branch from fd4a7c9 to 2dba05c Compare January 18, 2024 16:16
@ekhov ekhov marked this pull request as ready for review January 18, 2024 16:22
@ekhov ekhov force-pushed the support-multi-value-query-params branch 2 times, most recently from d0aa32a to f4a3e40 Compare January 20, 2024 17:34
@ekhov ekhov force-pushed the support-multi-value-query-params branch 2 times, most recently from 49af996 to b546a7b Compare January 20, 2024 22:24
@987Nabil
Copy link
Contributor

987Nabil commented Jan 21, 2024

@jdegoes The way this is modeled rn makes it impossible to to analyse if it is single or multi value. This might make it hard to generate docs etc. from endpoints. What do you think about adding a field queryStyle to the codec or a new annotation?
Both could also contain information about how multi values should be rendered. ?a=1&a=2 vs ?a=1,2 vs ?a=1|2 or maybe even others.
That would likely be more important for clients, to be compliant with their requirements.

@jdegoes
Copy link
Member

jdegoes commented Jan 22, 2024

@987Nabil The way this is modeled rn makes it impossible to to analyse if it is single or multi value.

How about QueryParamHint which could be QueryParamHint.Zero, QueryParamHint.One, QueryParamHint.Many, QueryParamHint.Any. Then the constructors can feed the hint in, and the documentation generation can use the hint.

Most code doesn't need to know about that, it's only really for doc or client generation.

cc @ekhov

@987Nabil
Copy link
Contributor

@ekhov I think you get the idea. Try what @jdegoes suggested and adjust the idea if needed. What is important is, that we can find intend in the data.

@ekhov
Copy link
Contributor Author

ekhov commented Jan 22, 2024

@jdegoes Do I get it correctly that you want this hint to be added as a Query parameter? And at the moment we will use only One(single value codecs) and Many(for multi value)?

ekhov added 6 commits January 22, 2024 19:27
QueryCodec now retuns NonEmptyChunk[A] instead of just A
Single value codecs that return A are derived from it
If query string specifies key fo expected query, but not the value,
parse it as empty chunk. If no keys are present- throw error
@ekhov ekhov force-pushed the support-multi-value-query-params branch 3 times, most recently from 9590049 to 446c5cd Compare January 22, 2024 18:40
@ekhov ekhov force-pushed the support-multi-value-query-params branch from 446c5cd to 8c376e2 Compare January 22, 2024 18:49
@ekhov ekhov requested a review from 987Nabil January 23, 2024 19:18
@jdegoes jdegoes merged commit a70a964 into zio:main Jan 24, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants